Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(deps): include "graphql" as a dependency #1381

Merged
merged 5 commits into from
Sep 1, 2022

Conversation

mattcosta7
Copy link
Contributor

@mattcosta7 mattcosta7 commented Aug 30, 2022

this makes the graphql peer dependency required. While the hope is to eventually avoid the need to require the graphql library for users who don't declare graphql requests directly, the current architecture means that many tools fails to properly compile without error/warning without this.

Since NPM 7, npm will automatically install peer dependencies, which should avoid consumers of msw having to manually install graphql.

While ideally we'd avoid this now - this PR may be a reasonable stopgap that avoids frustration/confusion in the interim

I also reverted the change to require('graphql'). The semantics of lazily requiring are a bit off imo, and given that it doesn't provide the desired effect, there's a bit more clarity maintaining import syntax.

A follow-up that separates the package/bundle in a way that allows to properly treeshake the graphql imports (or provide them from a different import specifier), so they aren't required dependencies would be ideal.

Totally get it, if the goal is to try a fail forward approach instead of trying to smooth out the current usage, until a more holistic fix can be merged - but figured this might avoid some annoyance in the meantime (both on consumers and avoid issues like #1371 and the handful of duplicates that have popped up over the last week)

this makes the graphql peer dependency required. While the hope is to eventually avoid the need to
require the graphql library for users who don't declare graphql requests directly, the current
architecture means that many tools fails to properly compile without error/warning without this.

Since NPM 7, npm will automatically install peer dependencies, which should avoid consumers of msw
having to manually install graphql.

re mswjs#1371
@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 30, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b26c2f1:

Sandbox Source
MSW React Configuration

removes the previous attempt at making graphql optional by lazily requiring it instead of importing
it.  This didn't have the intended affect of helping to avoid parsing in many environments, and
might be confusing and/or cause friction with a future migration to using native modules and
providing an export that can be treeshaken in all environments.

re mswjs#1371
@mattcosta7 mattcosta7 marked this pull request as ready for review August 30, 2022 12:38
@mattcosta7 mattcosta7 changed the title fix(package.json): make graphql non-optional peerDependency fix(graphql-peer-dep): make graphql non-optional peerDependency Aug 30, 2022
Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this, @mattcosta7!

I've left one proposal about how to move forward with this. I'm curious what you think about this.

@@ -1,6 +1,5 @@
import fetch from 'cross-fetch'
import { graphql as executeGraphql } from 'graphql'
import { buildSchema } from 'graphql/utilities'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use this from /utilities and from graphql in different files, so I moved to just using the base package export

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 agree with this. We should rely on the base package all the time.

@mattcosta7
Copy link
Contributor Author

Failing on typescript 4.5 looks like it was a network blip and I expect it to succeed if we re-run

@kettanaito
Copy link
Member

@mattcosta7, yeap, looks like a hiccup when installing dependencies on CI. Re-running...

@kettanaito kettanaito changed the title fix(graphql-peer-dep): make graphql non-optional peerDependency fix(deps): include "graphql" as a dependency Aug 30, 2022
Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good!

This should solve the dependency issue for those who're not using graphql. This should also not introduce any conflicts to those who are using graphql.

Let's test it out.

@kettanaito kettanaito merged commit 8436515 into mswjs:main Sep 1, 2022
@kettanaito
Copy link
Member

Thank you so much for your work on this, @mattcosta7! 👏 You're our dependency wizard.

@ashtonlance
Copy link

Thanks @mattcosta7!
@kettanaito When will this version be versioned and released?

@kettanaito
Copy link
Member

We need to write this somewhere.

@ashtonlance, MSW is released automatically at regular intervals. I believe it's every 3 days:

- cron: '0 0 */3 * *'

But since this is a blocking issue for our users I will trigger the release manually right now.

@kettanaito
Copy link
Member

Released: v0.46.1 🎉

This has been released in v0.46.1!

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tsc error: Cannot find module 'graphql' or its corresponding type declarations
3 participants